Skip to content

fix(unic-pr-review): stop silently dropping linked Work Items on large PRs#255

Merged
orioltf merged 8 commits into
developfrom
archon/task-feature-unic-pr-review-252-harden-work-item-discov
Jun 17, 2026
Merged

fix(unic-pr-review): stop silently dropping linked Work Items on large PRs#255
orioltf merged 8 commits into
developfrom
archon/task-feature-unic-pr-review-252-harden-work-item-discov

Conversation

@orioltf

@orioltf orioltf commented Jun 17, 2026

Copy link
Copy Markdown
Member

Summary

  • discoverWorkItems(prMetadata)discoverWorkItems(workItemRefs): takes the refs array directly, throws on undefined/non-array so data-loss can never collapse into the silent "no Work Items" case
  • ADO Fetcher hoists workItemRefs to a top-level FETCHER_OUTPUT field (summary tier, kept inline) instead of burying it inside the large prMetadata blob that gets abbreviated on big PRs
  • review-pr.md Step 1.5 now distinguishes absent key (data-loss → loud Notice + continue) from explicit [] (legitimate no-WI → silent)
  • ADR-0010 and ADR-0004 amended; CONTEXT.md Work Item definition updated; 699 tests green; patch bump 2.1.11 → 2.1.12

Closes #252

Why

PR #5647 (96 files, ~453 KB diff) had 3 linked Work Items (42602, 42610, 42611). The ADO Fetcher fetched them successfully but workItemRefs was buried inside the large prMetadata blob. The agent abbreviated the blob to keep its inline return small; workItemRefs was the casualty. The ?? [] guard then silently collapsed "data lost in transit" into "no Work Items linked", the Intent Check was never spawned, and the review ran with no intent coverage and no warning.

Test plan

  • pnpm --filter unic-pr-review test — 699 tests pass (0 fail)
  • pnpm --filter unic-pr-review typecheck — clean
  • New structural test (tests/work-item-discovery-contract.test.mjs) verifies review-pr.md pipes workItemRefs (not prMetadata) to discover-work-items and ado-fetcher.md Step 6 has workItemRefs at top-level indent
  • discoverWorkItems tests rewritten to refs-array signature; absent-key test flipped from → [] to → throws
  • gh pr checks (CI) — to verify after merge

🤖 Generated with Claude Code

orioltf and others added 5 commits June 17, 2026 10:04
…e PRs

discoverWorkItems now takes a workItemRefs array directly (not prMetadata),
throws on absent/non-array input, and the ADO Fetcher hoists workItemRefs
to a top-level FETCHER_OUTPUT field so it survives inline on large PRs.
review-pr.md Step 1.5 treats absent key as a data-loss Notice + continue
(third ADR-0004 state), not as a silent empty-WI result. ADR-0010 and
ADR-0004 amended; CONTEXT.md Work Item definition updated.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Format ADR-0004 markdown with Prettier (CI Biome+Prettier check)
- Normalise CRLF→LF in structural test to fix Windows Node test failures

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes:
- Update README.md discoverWorkItems section to new signature (HIGH)
- Update ADR-0001 amendment to new discoverWorkItems(workItemRefs) signature (HIGH)
- Add per-ref validation (id/url guards) in discoverWorkItems (LOW)
- Add CLI integration tests for discover-work-items subcommand (MEDIUM)
- Fix inaccurate comment about JSON blocks in contract test (LOW)
- Add depth-tracker multi-line assumption comment in contract test (LOW)
- Update AGENTS.md ADR-0004 one-liner with lost-in-handoff state (LOW)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@orioltf

orioltf commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-feature-unic-pr-review-252-harden-work-item-discov
Commit: 2155099
Philosophy: Fix everything unless clearly a new concern


Fixes Applied (7 total)

Severity Count
🔴 CRITICAL 0
🟠 HIGH 2
🟡 MEDIUM 1
🟢 LOW 4
View all fixes
  • README.md stale discoverWorkItems section (providers/azure_devops/README.md) — Updated heading, body, return-value semantics, and Adding Fixtures section to reflect new discoverWorkItems(workItemRefs) signature
  • ADR-0001 amendment stale signature (docs/adr/0001-multi-source-intent-with-shared-atlassian-credentials.md) — Replaced discoverWorkItems(prMetadata) with new signature, cross-referenced ADR-0010 amendment
  • No CLI tests for discover-work-items subcommand (providers/tests/index-cli.test.mjs) — Added 3 execFileSync tests: valid refs → normalised JSON, non-array stdin → exit 1, missing URL → exit 1
  • Missing per-ref validation in discoverWorkItems (providers/azure_devops/provider.mjs) — Added ref.id == null and typeof ref.url !== 'string' guards with descriptive error messages
  • Comment misstates "only json block in file" (tests/work-item-discovery-contract.test.mjs:76) — Replaced with accurate description of discriminating criterion
  • Depth-tracker edge case undocumented (tests/work-item-discovery-contract.test.mjs:98) — Added comment documenting multi-line assumption
  • AGENTS.md ADR-0004 one-liner missing lost-in-handoff state (AGENTS.md:70) — Extended to include absent-key → loud Notice + continue path

Tests Added

  • providers/azure_devops/tests/provider.test.mjs: throws on ref with missing id, throws on ref with null id, throws on ref with non-string url
  • providers/tests/index-cli.test.mjs: exits 0 for valid refs array, exits 1 for non-array stdin, exits 1 when URL missing

Skipped (2)

Finding Reason
Pre-existing Biome info warnings (6 in non-PR files) Pre-existing on develop, not in PR-touched files — separate housekeeping commit
Step 1.5 absent-key contract test Review agent recommended deferral (Option B) — throw guard is adequate backstop

Validation

✅ Type check | ✅ Lint (6 pre-existing infos, 0 errors) | ✅ Tests (705 passed, 6 new)


Self-fix by Archon · aggressive mode · fixes pushed to archon/task-feature-unic-pr-review-252-harden-work-item-discov

orioltf and others added 2 commits June 17, 2026 10:26
Both ado-fetcher describe tests computed extractJsonBlocks + .find
identically; extract to a shared const at the describe scope.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The simplify commit wrapped the find() call across multiple lines;
Biome's formatter requires it on one line at this width.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Summary

review-pr.md Step 1.5 sets NOTICES_CONTEXT.lostInHandoff on the work-item
data-loss path, but renderNotices() had no branch for it, so the durable
Summary Notice was silently dropped (the early terminal print still fired).
Add the render branch + typedef + unit tests so the Notice actually renders
at the top of the Review Summary and posts to the PR. Completes AC#5 of #252.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@orioltf orioltf merged commit 960e4ec into develop Jun 17, 2026
9 checks passed
@orioltf orioltf deleted the archon/task-feature-unic-pr-review-252-harden-work-item-discov branch June 17, 2026 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[unic-pr-review] Linked Work Items silently dropped on large PRs — Intent Check skipped with no warning

1 participant